-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Timefilter should return strings instead of moments #25625
Timefilter should return strings instead of moments #25625
Conversation
Pinging @elastic/kibana-app |
💔 Build Failed |
Looks like legit test failures. |
@lukasolson I'd like this PR to include a test which would currently fail. I think it probably needs to be a UI functional test. It might be easy at the end of one of the Discover tests to go change the dateFormat:tz from UTC to some other timezone (maybe some place that doesn't use daylight savings time like Arizona?) and then go back to discover and check the timestamp of a particular doc while using an absolute time range. With this bug it would probably fail everywhere except when run in that timezone. So maybe we need to pick a timezone where none of our team lives for the test... |
💔 Build Failed |
Please merge master to get CI working |
💔 Build Failed |
So I spent some more time digging today and it looks like this was introduced in #21827. Prior to that pull, the |
💔 Build Failed |
retest |
💚 Build Succeeded |
@@ -103,15 +103,15 @@ module.directive('kbnTimepicker', function (refreshIntervals) { | |||
return; | |||
} | |||
|
|||
_.set($scope, 'browserAbsolute.from', new Date(newDate.year(), newDate.month(), newDate.date())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (moment.isMoment(date) && $scope.mode === TIME_MODES.ABSOLUTE) { | ||
$scope.absolute.to = date; | ||
if (date && $scope.mode === TIME_MODES.ABSOLUTE) { | ||
$scope.absolute.to = moment.isMoment(date) ? date.toISOString() : date; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If date
is not a moment object, we simply return it. Could date
be a native javascript date? Seems we should ensure we're returning a string no matter what.
6b18c3e
to
f7bc474
Compare
@Bargs This is ready for another look |
@LeeDr I've added the functional tests. They test in multiple timezones so that it shouldn't pass/fail depending on which timezone you actually live in. |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM. Tested locally, compared to master, can verify that the issue is present on master but not in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
💚 Build Succeeded |
* fix timefilter to return strings instead of moment objects * add functional test * remove unused functions
* fix timefilter to return strings instead of moment objects * add functional test * remove unused functions
Fixes #25596.
Prior to this PR, if you had selected a time zone in your advanced settings other than your browser, then there was a possibility that the date histogram would show dates in your browser's time zone rather than the time zone you selected.
Part of the reason behind this is that the timefilter is inconsistent with what it returns in
getTime
. Sometimes it would return an object withfrom
as a UTC string, and other times it would return a moment object.There is some case that we're still debugging where the moment object gets stripped of its method properties, which in turns caused errors with the date histogram which attempts to call
moment(result)
with the resulting object, which in turn barfs inside the moment code because it's expecting function properties to be on the object which aren't actually there.--- Edit: See this comment which explains when that happens. --
As a follow-up PR, we should probably go through all of the places where we conditionally handle moment objects and remove it, since they are already handling the case when strings are passed.